[Nexthop][fboss2-dev] Replace file-based config versioning with Git#825
[Nexthop][fboss2-dev] Replace file-based config versioning with Git#825benoit-nexthop wants to merge 1 commit intofacebook:mainfrom
Conversation
c468783 to
81eaba1
Compare
3c9d00a to
af5398a
Compare
Summary: **Pre-submission checklist** - [x] I've ran the linters locally and fixed lint errors related to the files I modified in this PR. You can install the linters by running `pip install -r requirements-dev.txt && pre-commit install` - [x] `pre-commit run` Now every config command is saved in the CLI session metadata so we can easily tell what commands were used in a given session. The metadata is now also saved along the config when we commit the session. A future commit will make rollback also rely on this metadata to decide whether or not to restart the agent. Note: this change is part of a series, the previous one is #805, the next one is #825. Pull Request resolved: #809 Test Plan: New unit tests added. ## Sample usage ``` [admin@fboss101 ~]$ ~/benoit/fboss2-dev config interface eth1/1/1 description this is a test. Successfully set description for interface(s) eth1/1/1 [admin@fboss101 ~]$ ~/benoit/fboss2-dev config interface eth1/1/1 mtu 1500 Successfully set MTU for interface(s) eth1/1/1 to 1500 [admin@fboss101 ~]$ ls -l ~/.fboss2/ total 216 -rw-r--r-- 1 admin admin 213174 Jan 13 13:00 agent.conf -rw-r--r-- 1 admin admin 141 Jan 13 13:00 conf_metadata.json [admin@fboss101 ~]$ cat ~/.fboss2/conf_metadata.json { "action": {}, "commands": [ "config interface eth1/1/1 description this is a test.", "config interface eth1/1/1 mtu 1500" ] } [admin@fboss101 ~]$ ~/benoit/fboss2-dev config session commit Config session committed successfully as r6 and config reloaded. [admin@fboss101 ~]$ ls -l /etc/coop/cli/ total 1292 -rw-r--r-- 1 root root 213185 Jan 13 12:52 agent-r1.conf -rw-r--r-- 1 admin admin 213194 Jan 13 12:56 agent-r2.conf -rw-r--r-- 1 admin admin 108 Jan 13 12:56 agent-r2.metadata.json -rw-r--r-- 1 admin admin 213185 Jan 13 12:56 agent-r3.conf -rw-r--r-- 1 admin admin 99 Jan 13 12:56 agent-r3.metadata.json -rw-r--r-- 1 admin admin 213185 Jan 13 12:58 agent-r4.conf -rw-r--r-- 1 admin admin 80 Jan 13 12:58 agent-r4.metadata.json -rw-r--r-- 1 admin admin 213185 Jan 13 12:58 agent-r5.conf -rw-r--r-- 1 admin admin 80 Jan 13 12:58 agent-r5.metadata.json -rw-r--r-- 1 admin admin 213174 Jan 13 13:00 agent-r6.conf -rw-r--r-- 1 admin admin 141 Jan 13 13:00 agent-r6.metadata.json [admin@fboss101 ~]$ cat /etc/coop/cli/agent-r6.metadata.json { "action": {}, "commands": [ "config interface eth1/1/1 description this is a test.", "config interface eth1/1/1 mtu 1500" ] } ``` Reviewed By: KevinYakar Differential Revision: D93669865 Pulled By: joseph5wu fbshipit-source-id: 9c37dfd88a59d8e76bdb09f30d0c55897c4e889f
af5398a to
d4f6898
Compare
Replace the basic file-based configuration versioning mechanism with Git-based versioning for the CLI config session. Key changes: - Add new Git class (Git.h/cpp) providing a simple interface for Git operations: init, commit, log, show, resolveRef, getHead, hasCommits - Use folly::Subprocess with full path /usr/bin/git for all Git commands - Replace revision files (agent-rN.conf + symlink) with atomic writes to agent.conf tracked in a local Git repository - Use Git commit SHAs as revision identifiers instead of rN format - Update RevisionList validation to accept Git SHAs (7+ hex chars) Repository initialization: - Automatically initialize Git repo if it doesn't exist - Automatically create initial commit if repo has no commits but config file exists - Use --shared=group flag and umask 0002 to ensure .git directory is group-writable when /etc/coop is group-writable Commands updated: - config history: Shows Git commit log with SHA, author, timestamp, message - config session diff: Uses git show to compare commits - config session commit: Creates Git commits with username as author - config rollback: Reads config from Git history and creates new commit Test updates: - Update all CLI config tests to use Git-based setup - Initialize Git repo and create initial commit in test fixtures
d4f6898 to
b8b37f2
Compare
|
@joseph5wu has imported this pull request. If you are a Meta employee, you can view this in D94975358. |
joseph5wu
left a comment
There was a problem hiding this comment.
There're some improvement comments about how to properly manage the full sha vs short sha, to avoid mis-using them in your code. Please consider to improve the logic in the future PRs.
I'll land this PR for now to unblock your rebase effort
|
|
||
| // Build message based on what actions were taken | ||
| std::string message; | ||
| std::string shortSha = result.commitSha.substr(0, 7); |
There was a problem hiding this comment.
Would like to see a common utility function for getShortSha(const std::string& commitSha).
Or a more organized way to getFullSha() or getShortSha() from the result.
This will avoid mis-using the wrong sha string.
Could be addressed in future diff
There was a problem hiding this comment.
Just to be clear, you can always use the long or the short sha1, as long as it's a unique prefix (just like when using git directly). But other than that, yes, we can add a little helper function to clip the string.
| throw std::runtime_error( | ||
| "Failed to read current config from " + cliConfigPath); | ||
| } | ||
| return {content, "current live config"}; |
There was a problem hiding this comment.
Will it be a problem that you don't give a shortSha of the current revision here?
In line 42, you always return shortSha revision for non-current case.
There was a problem hiding this comment.
You can pass anything that git will accept: short sha1, full sha1, branch or tag name, any other ref...
| std::string content = git.fileAtRevision(resolvedSha, "cli/agent.conf"); | ||
| return {content, revision.substr(0, 8)}; |
There was a problem hiding this comment.
Now I think your Git can have to provide a getShortSha()
There was a problem hiding this comment.
Yes it's inconsistent with the other file, good catch. It doesn't matter in practice but I agree it would be better to be consistent. We can fix this in this PR if you want, it's such a small change it wouldn't hurt anything.
|
@joseph5wu merged this pull request in 307355c. |
…esolution (#832) Summary: **Pre-submission checklist** - [x] I've ran the linters locally and fixed lint errors related to the files I modified in this PR. You can install the linters by running `pip install -r requirements-dev.txt && pre-commit install` - [x] `pre-commit run` When multiple users have concurrent config sessions, the first user to commit succeeds, but subsequent users get an error because their session is based on a stale commit. Previously, whoever committed last would overwrite changes of other users that committed before them. This commit adds a `config session rebase` command that: 1. Takes the diff between the config as of the base commit and the session 2. Re-applies this diff on top of the current HEAD (similar to git rebase) 3. Uses a recursive 3-way merge algorithm for JSON objects 4. Detects and reports conflicts at specific JSON paths 5. Updates the session's base to the current HEAD on success The 3-way merge algorithm handles: - Objects: recursively merges each key - Arrays: element-by-element merge if sizes match - Scalars: conflict if both head and session changed differently from base Note: this change is part of a series, the previous one is #825, the next one is #837. Pull Request resolved: #832 Test Plan: Add unit tests covering: - Successful rebase with non-conflicting changes - Rebase failure when changes conflict - Rebase not needed when session is already up-to-date ## Sample usage TBD Reviewed By: srikrishnagopu Differential Revision: D95099578 Pulled By: joseph5wu fbshipit-source-id: b665afa35a43829981ce9dd8434abf5b3942882a
Pre-submission checklist
pip install -r requirements-dev.txt && pre-commit installpre-commit runSummary
Replace the basic file-based configuration versioning mechanism with Git-based versioning for the CLI config session.
Key changes:
Gitclass providing a simple interface for Git operations: init, commit, log, show, etcfolly::Subprocesswith full path/usr/bin/gitfor all Git commandsagent-rN.conf+ symlink) with atomic writes to agent.conf tracked in a local Git repositoryRepository initialization:
--shared=groupflag and umask 0002 to ensure.gitdirectory is group-writable when/etc/coopis group-writableCommands updated:
Note: this change is part of a series, the previous one is #809, the next one is #832.
Test Plan
Test updates:
Sample usage
Simple change and session commit:
Rollback flow: